Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Allow use of pipefail where supported #1008

Merged
merged 1 commit into from
Jun 17, 2022
Merged

Allow use of pipefail where supported #1008

merged 1 commit into from
Jun 17, 2022

Conversation

fredden
Copy link
Contributor

@fredden fredden commented Jun 9, 2022 •
edited
Loading

Magerun pull-request check-list:

  • Pull request against develop branch (if not, just close and create a new one against it)
  • README.md reflects changes (if any)
  • phar fuctional test (in tests/phar-test.sh)

Summary: Allow use of pipefail where supported

This is related to #929. It doesn't fix it fully, but does make the symptom go away a lot of the time.

Changes proposed in this pull request:

This pull request removes the check for "bash compatible" shells, relying on the actual feature check for the feature we want to use.

We use docker heavily, and in our set-up, when we run PHP commands there is no SHELL environment variable, so the "is this shell bash-compatible" check does not pass. Removing this check allows the pipefail feature detection to actually work. I've tested this with shells that do and do not support the feature, and the existing feature detection responds as expected. We're having trouble where command failures aren't being detected in our CI/CD platform because of this bug.

@cmuench
Copy link
Member

cmuench commented Jun 9, 2022

@fredden I already spent hours to find a solution for #929 which is quite complicated.
The db:dump command needs some refactoring.
I am not happy with isBashCompatibleShell function. If we remove the function completly, we could btw. break 3rd party magerun modules.

@fredden
Copy link
Contributor Author

fredden commented Jun 9, 2022 •
edited
Loading

@cmuench should I re-add / not remove that function then? I think removing its usage within Utils\Exec::run() is still correct.

Would you like it marked as @deprecated so it can be safely removed in a future version?
Would you prefer a new commit re-adding the method, or a force-push so it's never touched/modified here?

@cmuench
Copy link
Member

cmuench commented Jun 9, 2022

@cmuench should I re-add / not remove that function then? I think removing its usage within Utils\Exec::run() is still correct.

Would you like it marked as @deprecated so it can be safely removed in a future version? Would you prefer a new commit re-adding the method, or a force-push so it's never touched/modified here?

That's a good idea to mark the method as deprecated. 👍

@cmuench
Copy link
Member

cmuench commented Jun 9, 2022

@cmuench should I re-add / not remove that function then? I think removing its usage within Utils\Exec::run() is still correct.
Would you like it marked as @deprecated so it can be safely removed in a future version? Would you prefer a new commit re-adding the method, or a force-push so it's never touched/modified here?

That's a good idea to mark the method as deprecated. 👍

You can squash the commits together.

@fredden
Copy link
Contributor Author

fredden commented Jun 9, 2022

Thanks for the feedback @cmuench. I've updated the pull request as discussed.

@cmuench cmuench merged commit d0d9f14 into netz98:develop Jun 17, 2022
@fredden fredden deleted the pipefail branch June 17, 2022 14:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@fredden @cmuench